-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove send_destination_prefix #118
Conversation
Remove the use of the send_destination_prefix attribute from yggd's busconfig policy. Instead, each worker must install their own busconfig policy. Update the echo worker to demonstrate this. Signed-off-by: Link Dupont <[email protected]>
621c1a8
to
454370c
Compare
Use the old systemd pkgconfig variable names for systemunitdir and userunitdir. This supports systemd back to at least 239. Signed-off-by: Link Dupont <[email protected]>
Since this service isn't associated with a systemd unit, specify the user that the service should run as. Signed-off-by: Link Dupont <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that proper testing of such change is just blocked by this issue #121
Missing an RPM should not block the ability for the software to be tested and run. An RPM is just a delivery method of the artifacts built by the system. You can also install from the source tree with meson: |
@@ -13,3 +13,7 @@ configure_file( | |||
install: true, | |||
install_dir: dbus.get_variable(pkgconfig: 'system_bus_services_dir') | |||
) | |||
|
|||
install_data('com.redhat.Yggdrasil1.Worker1.echo.conf', | |||
install_dir: join_paths(dbus.get_variable(pkgconfig: 'datadir'), 'dbus-1', 'system.d') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to install this in a RHEL9.2 (also in RHEL8.7)
I couldn't see this files installed:
cat meson-logs/install-log.txt | grep "Worker1"
/usr/share/dbus-1/interfaces/com.redhat.Yggdrasil1.Worker1.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you configure the project to install the examples? meson setup -Dexamples=True
. By default, it does not install the example echo
worker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not, let me check it with that option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I tested in rhel9.2 and rhel8.7 echo worker and yggdrasil are performing as they should
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I have only a few questions.
systemd_system_unit_dir = systemd.get_variable(pkgconfig: 'systemd_system_unit_dir') | ||
systemd_user_unit_dir = systemd.get_variable(pkgconfig: 'systemd_user_unit_dir') | ||
systemd_system_unit_dir = systemd.get_variable(pkgconfig: 'systemdsystemunitdir') | ||
systemd_user_unit_dir = systemd.get_variable(pkgconfig: 'systemduserunitdir') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you do this change? Is it necessary? It is easier to read systemd_system_unit_dir
then systemdsystemunitdir
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this PR is to enable backwards compatibility to EL8. To that end, I found that the version of systemd shipping in EL8 does not include the underscore-separated variable names. This change is necessary to support systemd versions older than 239.
fix: use old systemd pkgconfig variable names
Use the old systemd pkgconfig variable names for systemunitdir and
userunitdir. This supports systemd back to at least 239.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
@@ -1,3 +1,4 @@ | |||
[D-BUS Service] | |||
Name=com.redhat.Yggdrasil1.Worker1.echo | |||
User=root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to run this worker as a root? Or is it necessary to run all workers as a root? Could it be changed in the future? It would be more secure to run service as non-root user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version of D-Bus shipping in EL8 requires a User=
directive. To support the current behavior of starting this service automatically on the system bus, I set the user to root.
"-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN" | ||
"https://dbus.freedesktop.org/doc/busconfig.dtd"> | ||
<busconfig> | ||
<policy user="root"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is root user really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bus policy only applies to the service when running on the system bus. The current implementation assumes that both yggd and workers run as root, so the policy is applied to the root user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Remove the use of the send_destination_prefix attribute from yggd's
busconfig policy. Instead, each worker must install their own busconfig
policy. Update the echo worker to demonstrate this.
Signed-off-by: Link Dupont [email protected]